The text only applies if nodes set var_onion_optin to required, it makes no recommendation as to when a node should do that.

IMHO it makes no sense to require var_onion_optin today because you'd be losing a lot of connectivity. But that's something that could make sense if we find a security flaw in the legacy format for example. A node that requires var_onion_optin will simply refuse to relay/receive HTLCs with the legacy format. As a sender, when you're building your payment route, if some of the nodes in the route require var_onion_optin you should either use the tlv format (if you support it), or not include that node in your payment route. I think that Bolt 4 correctly specifies that behavior, do you feel otherwise?

Note that “connect to” a node has nothing to do with routing a payment to. You, in fact, usually route payments to nodes you aren’t connected to!

Got it, we didn't understand "route to" the same way then (depending on the context, it may mean different things so we should clarify that!).
In Bolt 2 and Bolt 7 it's always used as "routing a payment through a channel", so in my mind it always means forwarding HTLCs or gossip to a connected peer.
If what you meant is "including the node in a payment route" I think those two requirements aren't needed (already captured in Bolt 4).

Does that make sense or am I missing something?

a) This makes no judgement about requiring var_onion_optin or not. There's a bit for supports, and a bit for required, this just clarifies what those bits, in general mean, lets not debate what bits nodes should be setting. b) More importantly, however, this change clarifies what required unknown bits mean today - when we add better_variable_length_onion or other_amp or point_based_relay, we need to know how to interpret the unknown bits as now-"legacy" nodes.

Then back to the PR content: we should set var_onion_optin to IN9 in Bolt 9, this was an oversight. IMO this should be done in #719 which adds more to that area of the spec.

You didn't answer my feedback/comments on the proposed Bolt 7 changes.
Either you're connected to the node, and the existing requirement feels enough to me.
Or you're not connected to the node, and the only case where you're looking at that node's features is when building a payment route; in that case Bolt 4 covers today's behavior (for var_onion_optin). If we want to future-proof it, IMO it's in Bolt 4 that we should add a requirement saying that a writer "MUST NOT include a node in the payment route if this node advertised an unknown even feature".

It feels out of place in Bolt 7 (which is supposed to describe how you receive and relay gossip messages and graph data) and the wording is confusing, "routing messages" seems to indicate we can decide through which nodes our gossip goes through whereas in reality it only applies to payments.

We can move it to BOLT 4 if you like, but then we should probably move the whole "unknown features" clause there as well. Mostly, it doesn't matter much where it goes, it just needs to go somewhere. BOLT 4 isn't obviously correct here to me, nor is BOLT 7.

In general, it sounds like, however, you agree with both changes, just maybe not where in the doc they go, correct?

As for fixing the BOLT 4/9 conflict, I don't care who takes that commit. If Connor wants to take it in 719, great, if not, it can go in here!

Yes, I think these changes make sense, it's just that I found the wording and location confusing (but maybe that's just me).
Bolt 7 has been used a bit too much as the place where everything goes when we don't really know where to put it.

For the proposed changes, I think https://github.com/lightningnetwork/lightning-rfc/blob/master/04-onion-routing.md#packet-construction is a good location: that's where we explain the multi-hop routing and packet construction, so I believe it would fit well.

I agree that maybe we should move the "SHOULD NOT connect to the node." feature requirement. Since it's a requirement on whether to init a connection to that node or not, maybe a better place would be here: https://github.com/lightningnetwork/lightning-rfc/blob/master/01-messaging.md#the-init-message?

Maybe @rustyrussell also has something to suggest for that?

If you find it confusing, you have a proposed alternate wording?

I'd change it to something like:

• Unless paying a BOLT #11 invoice which does not have
  the same bit(s) set, MUST NOT send payments to the node.
• MUST NOT include the node in a payment route.

I'm also wondering if it really should be a "MUST NOT". I think SHOULD NOT is enough.
Just like you may choose to connect to a node with unknown even features (if for example you checked online that those features shouldn't really impact your interactions with that node), you may choose to include nodes with unknown even features in your payment routes. If those nodes really can't or don't want to relay your HTLCs, they'll send an error and you can retry with another route.

Sorry if you feel I'm nitpicking, I promise the goal isn't to be annoying :)
I went through the process of reading the RFC to ramp up from 0 a bit less than a year ago, and it was honestly not that simple because many small requirements turned out to be at times very confusing or more subtle than they seemed.
I expect many newcomers to come to the RFC as a reference, and anything we can do to make it even slightly less confusing is worth it IMO. A bit more work for a few people during the PR process, but in the end it's a lot less time spent overall when we consider all the others who will read that spec and try to understand it or implement it.

I tweaked the wording to be closer to your proposed, is that a bit better?

As for MUST NOT/SHOULD NOT - I think in your described case, MUST NOT still applies fine - if you checked what those features mean, its no longer an "unknown" feature! In general, unless there's a good reason for it, we should prefer MUST NOT over SHOULD NOT - Postel's law has been pretty generally thrown out as bad advice when attempting to build robust systems.

ACK 5abee4d

Ping @cfromknecht :-)

Clients should be looking at the feature bits flags in the channel ann and channel updates instead of solely relying on node announcements. Consider the scenario in the future where a new channel type is added (or a modification on an existing type like anchor outputs). A node wishes to signal that it'll only consider new incoming channel requests that use this new channel type. In order the achieve this, they set the feature bit as required in the node announcement. This preference has no bearing on if a payment should be routed through this node (in this simplified scenario) as it expresses channel acceptance preference.

Clients should be looking at the feature bits flags in the channel ann and channel updates

channel updates don't have feature bits

instead of solely relying on node announcements.

I don't see where this says to only rely on node_announcements? There is already text there to say you cannot route through a node based on their channel_announcement features.

. A node wishes to signal that it'll only consider new incoming channel requests that use this new channel type

Right - this is somewhat confused in the current doc. Probably makes more sense to continue this discussion at #726, but var_onion_optin is a node_feature, which makes no sense in your interpretation of what node_features are for.

I don't see where this says to only rely on node_announcements? There is already text there to say you cannot route through a node based on their channel_announcement features.

It may not say it explicitly, but the guidelines here can result in degraded routing for a client which blindly adheres to them.

but var_onion_optin is a node_feature, which makes no sense in your interpretation of what node_features are for.

My example wasn't meant to be exhaustive. Instead, it shows a plausible and upcoming instance where a required feature bit on a node announcement doesn't restrict the set of HTLCs that can be routed through it. The TLV onion bit is a node level feature as onion payload are largely independent of channels, it's the node that needs to understand the payload.